feat(cli): add support for poll watcher #21290
Conversation
|
|
amribm
force-pushed
the
feature/support_for_poll_watcher
branch
from
3773b14
to
ae66312
Compare
amribm
force-pushed
the
feature/support_for_poll_watcher
branch
from
4c926d0
to
1802ece
Compare
amribm
changed the title
Co-authored-by: Jorge Hermo <[email protected]>
|
Hey @amribm, thank you for this PR!
|
|
okay, i will add changelog
Yes This is ready for review , @pront i need some confirmation from vector team for those comments. that's why i didn't resolved them. |
Co-authored-by: Pavlos Rontidis <[email protected]>
Co-authored-by: Pavlos Rontidis <[email protected]>
Co-authored-by: Pavlos Rontidis <[email protected]>
… and docs update
github-actions
bot
added
the
domain: external docs
Co-authored-by: Jorge Hermo <[email protected]>
| @@ -19,11 +22,49 @@ const CONFIG_WATCH_DELAY: std::time::Duration = std::time::Duration::from_secs(1 | |||
|
|
|||
| const RETRY_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); | |||
|
|
|||
| /// Refer to [`crate::cli::WatchConfigMethod`] for details. | |||
| pub enum WatcherConfig { | |||
There was a problem hiding this comment.
I wonder if we can refactor create_watcher to use the WatchConfigMethod enum directly. AFAIK, due to clap parsing, the WatchConfigMethod needs to have plain enum variants.
There was a problem hiding this comment.
Since i want that enum to have values about the interval and i don't want to pass the interval in the function parameters.
One thing we can do is we can eliminate is watch config method enum. we can make it as string and add the possible values for clap arguments. using that we can create the WatcherConfig enum
Co-authored-by: Jorge Hermo <[email protected]>
| } | ||
|
|
||
| fn watch(&mut self, path: &Path, recursive_mode: RecursiveMode) -> Result<(), Error> { | ||
| use notify::Watcher as NotifyWatcher; |
There was a problem hiding this comment.
nit: any reason to use this here vs at the top like the other imports?
There was a problem hiding this comment.
Actually we already have name watcher as enum. it was colliding with watcher from notify.
Renaming the import at the top level user might not able to understand what is NotifyWathcher and we need the methods for notify::Watcher on that specific scope. that's why i used Watcher on the specific scope
pront
changed the title
|
I think we addressed all open questions here? cc @amribm and @jorgehermo9 |
|
LGTM! |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
|
Apologies, I didn't realize this got pushed out of the merge queue. |
|
When this PR will merge @pront? |
Signed-off-by: Jesse Szwedko <[email protected]>
It looks like there were cue formatting issues blocking merge as the failed CI check shows. I formatted and pushed though so I think this should merge soon if there are no other failing checks. |
|
Thanks again, this is a great contribution 🚀 |
|
Thank you guys, this is my first open-source contribution. thanks for making this possible. @jorgehermo9 @pront @brittonhayes |
Closes #21111